Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MDCMigration: Style runs data table row heights for MDC migration. #6616

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

bmd3k
Copy link
Contributor

@bmd3k bmd3k commented Oct 3, 2023

Restyle the height of header and rows in the runs data table.

The height of these rows does need to increase compared to current master in order to account for the increase in height of the checkbox touch target. But they don't need to be as tall as they are now compared to current MDCMigration.

The end result is:

  • Header and rows height are now 48px. (compared to 56px and 49px in MDCMigration and compared to 48px and 44px in master). So the non-header rows grow in height by 4px compared to master. Header rows remain the same compared to master.

Implementation Detail:

  • I move the common padding: 4px out of content_cell_component and header_cell_component and now make this padding the responsibility of runs_data_data and scalar_card_data_table to specify.
  • scalar_card_data_table specifies, for the time being, padding: 4px until we determine we need to make adjustments to it as well for MDC migration.
  • runs_data_table specifies different padding and adjust its height.

Here is a sample screenshot but please patch it in and play with it:

image

@bmd3k bmd3k requested a review from JamesHollyer October 3, 2023 18:21
@@ -48,11 +48,13 @@ $_arrow_size: 16px;

tb-data-table-content-row,
tb-data-table-header-cell {
height: 44px;
// Match the size of the touch targets for the checkbox.
height: 48px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish the touch target would match our size like all the other parts of mdc-checkbox do. I am ok with this though.

@bmd3k bmd3k merged commit 33dcfc0 into tensorflow:MDCMigration Oct 4, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants